-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prefix custom event names #257
Conversation
Because HTML attributes are case insensitive, browsers interpret any uppercase characters as lowercase. This can cause problems across frameworks. As such, for standardized usage we use lowercased kebab-style formatting for custom events.
98b3cdc
to
2d5f6da
Compare
Regarding event naming conventions, I have been using the convention of I think it's a useful convention, mostly because it reduces noise: subscribers only ever receive events for topics that they are subscribed to. But also because clients then do not have to check that ids match if they are only interested in changes for a particular document or node within a document. Regardless of the change to kebab case, I think we should use the "subject-verb" convention ie
When using @Listen('stencila-node-patched', { target: 'window' }) To get around this, I suggest that the web client subscribe to the server events using the /**
* Subscribe to a document topic
*/
export async function subscribe(
client: Client,
documentId: DocumentId,
topic: DocumentTopic,
handler: (event: DocumentEvent) => void
): Promise<Document> {
client.on(`documents:${documentId}:${topic}`, handler)
return client.call('documents.subscribe', {
documentId,
topic,
}) as Promise<Document>
} but that the export interface NodePatched {
nodeId: DocumentId
patch: Patch
} Currently the |
There are a couple of different points here that I'd like to untangle, mainly event names and behaviours in the
I agree, and I had also followed your example in the component listeners for the patch events.
This is where I think the context is different.
This gets to the second issue of ergonomics with DX. // MyComponent.tsx in pseudo code follows
// with kebab case
// ... some boilerplate
render () {
return <stencila-article onStencila-documents-patch={e => console.log(e.detail.documentId)}></stencila-article>
}
// with colons, one possible solution
// ... some boilerplate
MyArticle = () => {
const article = <stencila-article></stencila-article>
article[`onStencila-document:patch:${topic}`] = (e) => console.log(e.detail)
}
render () {
return <MyArticle></MyArticle>
} Key being that we lose the ability to set the handler as props/attributes on the components.
For performance reasons it's better to use as few listeners as possible, so having a the subjects in there makes it difficult, and a little redundant as the emitted event contains the target.
This one I have a more of an opinion, and would prefer to stick to standard DOM Event convention e.g. ( With that in mind though, I'll try and rename the events to include your feedback |
What events the each page web client receivesThis is not what is happening at present 👇🏽
This is what is happening at present 👇🏽
☝🏽 That is what I meant by "subscribe to the server events using the id of the document as it currently does". In other words, each web client will only receive events for the topic, which includes the client.on(`documents:${documentId}:${topic}`, handler) The Naming conventions
I suspect you are taking a strict interpretation of "subject" (i.e. a unique identifier for an instance) and perhaps didn't look at my examples e.g. |
BREAKING CHANGE: Event names have changed, please refer to the component README for updated versions.
BREAKING CHANGE: Please use `programmingLanguage` prop, or `programming-language` HTML attribute instead.
748d6e5
to
4527944
Compare
This PR closes #253. It:
@stencila/dev-config